Skip to content

🧪 Add unit tests for SettingsRepository - #190

Closed
alvin000009238 wants to merge 1 commit into
mainfrom
jules-5137358925869291418-b28e8fad
Closed

🧪 Add unit tests for SettingsRepository#190
alvin000009238 wants to merge 1 commit into
mainfrom
jules-5137358925869291418-b28e8fad

Conversation

@alvin000009238

Copy link
Copy Markdown
Owner

🎯 What:
Implemented a unit test suite for SettingsRepository to cover the previously missing test gap. Refactored SettingsRepository slightly to accept a DataStore<Preferences> parameter for easier dependency injection during testing while keeping the Context-based constructor for backwards compatibility.

📊 Coverage:

  • testDefaultSettings: Validates that the default values returned by the repository match expectations.
  • testUpdateSettings: Validates that all setter functions correctly apply the corresponding configuration changes to the DataStore.
  • testMultipleUpdates: Uses Turbine to confirm that the Flow<AppSettings> correctly emits updated values consecutively.

Result:
Significant improvement in reliability and coverage for local persistence logic in SettingsRepository.


PR created automatically by Jules for task 5137358925869291418 started by @alvin000009238

Co-authored-by: alvin000009238 <107313913+alvin000009238@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

Copilot AI review requested due to automatic review settings June 17, 2026 14:30

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors SettingsRepository to support dependency injection by accepting DataStore<Preferences> in its primary constructor, and introduces a comprehensive suite of unit tests in SettingsRepositoryTest.kt. The review feedback correctly identifies an anti-pattern where a single TestScope is shared across multiple tests, which can lead to test interference or failures. It is recommended to isolate each test's scope using runTest(testDispatcher) and to safely resolve the temporary file path using File(tempFolder.root, ...) inside the PreferenceDataStoreFactory configuration.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +23 to +32
private val testDispatcher = UnconfinedTestDispatcher()
private val testScope = TestScope(testDispatcher)

private fun createRepository(): SettingsRepository {
val dataStore = PreferenceDataStoreFactory.create(
scope = testScope,
produceFile = { tempFolder.newFile("test_settings.preferences_pb") }
)
return SettingsRepository(dataStore)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Sharing a single TestScope instance across multiple tests via a class property is an anti-pattern in kotlinx.coroutines.test. When a test finishes, the TestScope is completed/cancelled, which will cause subsequent tests using the same scope to fail or behave unpredictably.

Instead, each test should run in its own isolated TestScope by calling runTest(testDispatcher). We can refactor createRepository to be an extension function on TestScope so it can use the current test's scope.

Additionally, calling tempFolder.newFile(...) inside the produceFile lambda of PreferenceDataStoreFactory.create can throw an IOException if the file already exists or is initialized multiple times. It is safer to resolve the file path using File(tempFolder.root, ...) instead.

    private val testDispatcher = UnconfinedTestDispatcher()

    private fun TestScope.createRepository(): SettingsRepository {
        val dataStore = PreferenceDataStoreFactory.create(
            scope = this,
            produceFile = { File(tempFolder.root, "test_settings.preferences_pb") }
        )
        return SettingsRepository(dataStore)
    }

}

@Test
fun testDefaultSettings() = testScope.runTest {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use runTest(testDispatcher) instead of the shared testScope.runTest to ensure each test runs in its own isolated TestScope.

Suggested change
fun testDefaultSettings() = testScope.runTest {
fun testDefaultSettings() = runTest(testDispatcher) {



@Test
fun testUpdateSettings() = testScope.runTest {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use runTest(testDispatcher) instead of the shared testScope.runTest to ensure each test runs in its own isolated TestScope.

Suggested change
fun testUpdateSettings() = testScope.runTest {
fun testUpdateSettings() = runTest(testDispatcher) {

}

@Test
fun testMultipleUpdates() = testScope.runTest {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Use runTest(testDispatcher) instead of the shared testScope.runTest to ensure each test runs in its own isolated TestScope.

Suggested change
fun testMultipleUpdates() = testScope.runTest {
fun testMultipleUpdates() = runTest(testDispatcher) {

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds JVM unit test coverage for SettingsRepository’s local DataStore persistence and refactors the repository constructor to support dependency injection while preserving the existing Context-based construction path.

Changes:

  • Introduced SettingsRepositoryTest covering defaults, individual updates, and consecutive Flow<AppSettings> emissions (Turbine).
  • Refactored SettingsRepository to accept DataStore<Preferences> directly, with a secondary Context constructor for compatibility.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
android/app/src/test/java/com/clhs/score/data/SettingsRepositoryTest.kt New unit test suite validating SettingsRepository default values and update behavior, including sequential Flow emissions.
android/app/src/main/java/com/clhs/score/data/SettingsRepository.kt Constructor refactor to allow injecting a DataStore<Preferences> while keeping the Context constructor.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +3 to +5
import androidx.datastore.preferences.core.PreferenceDataStoreFactory
import java.io.File
import kotlinx.coroutines.flow.first

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4cc2c5003c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


private fun createRepository(): SettingsRepository {
val dataStore = PreferenceDataStoreFactory.create(
scope = testScope,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use backgroundScope for test DataStore jobs

When these tests collect repository.settings, DataStore 1.1+ starts long-lived work in the scope passed to PreferenceDataStoreFactory.create; because that scope is the same TestScope that runs each runTest and is never cancelled as background work, runTest will wait for the active DataStore job and fail or time out with UncompletedCoroutinesError. Pass the current test's backgroundScope into the factory (or cancel a separate DataStore scope in teardown) so the DataStore job is cleaned up after each test.

Useful? React with 👍 / 👎.

@alvin000009238
alvin000009238 deleted the jules-5137358925869291418-b28e8fad branch July 9, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants